Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Proposal: Include Client Address in PSK Validation for Brute Force Detection #596

Closed
wants to merge 7 commits into from
Closed

Proposal: Include Client Address in PSK Validation for Brute Force Detection #596

wants to merge 7 commits into from

Conversation

tonisole
Copy link

Description

I am proposing a modification to the current PSK validation mechanism to include the client's address as part of the validation process. This change is crucial for implementing a Brute Force Detection mechanism in our system.

Currently, the PSK validation does not provide any information about the client attempting to connect. This lack of information makes it impossible to detect if a specific IP address is repeatedly trying to discover the Hint/PSK authorization, a common sign of a brute force attack.

var attempts = make(map[string]int) // Map of attempts for each IP address for a Brute Force Protection

PSK: func(hint []byte, addr net.Addr) ([]byte, error) {
	fmt.Printf("Server's hint: %s \n", hint)
	// *************** Brute Force Attack protection ***************
	// Check if the IP address is in the map, and the IP address has exceeded the limit
	if attempts[addr.(*net.UDPAddr).IP.String()] > 5 {
		return nil, fmt.Errorf("too many attempts from this IP address")
	}
	// Here I increment the number of attempts for this IP address
	attempts[addr.(*net.UDPAddr).IP.String()]++

I am using this library in conjunction with the go-coap library, and having the ability to implement this detection mechanism would significantly enhance the security of our system. By including the client's address in the PSK validation, we can monitor and limit the number of attempts from a single IP address, effectively preventing potential brute force attacks.

This draft is intended to open a discussion about the necessity and feasibility of this proposed change. I believe that with this modification, we can make our system more secure and robust against potential threats. I look forward to hearing your thoughts and suggestions on this matter.

Reference issue

No related issue

@tonisole
- Minimum changes to be able to get the IP address in the UDP-PSK
7f0f55b 
handshake function to implement a Brute Force Attack protection.
@tonisole
- Updated the PSK example to implement a Rate Limiting Brute
7514727 
Force Attack to Limit the number of requests a single IP address
can make in a certain amount of time. If an IP address exceeds this
limit it temporarily ban it.
@codecov Codecov
Copy link

codecov bot commented Nov 22, 2023 •
edited
Loading

Codecov Report

Attention: Patch coverage is 66.66667% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 79.98%. Comparing base (adec94a) to head (3e7a2ef).

Files Patch % Lines
flight4handler.go 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #596      +/-   ##
==========================================
- Coverage   80.02%   79.98%   -0.04%     
==========================================
  Files         101      101              
  Lines        5306     5306              
==========================================
- Hits         4246     4244       -2     
- Misses        685      687       +2     
  Partials      375      375
Flag Coverage Δ
go 80.01% <66.66%> (-0.04%) ⬇️
wasm 63.85% <66.66%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

at-wat
at-wat reviewed Nov 22, 2023
View reviewed changes
Copy link
Member

@at-wat at-wat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really nice to have this feature, however, it requires major version bump as it's a breaking change.
How about keeping PSK: func([]byte) ([]byte, error) and adding PSKWithAddr: func([]byte, net.Addr) ([]byte, error)?

@Sean-Der
Copy link
Member

@at-wat @tonisole We do need to tag a /v3!

@hasheddan can I get a short write up on your changes? I say we do this change proposed here, and yours? I really would like to get pion/dtls stabilized soon :)

hasheddan
hasheddan reviewed Nov 27, 2023
View reviewed changes
Copy link
Contributor

@hasheddan hasheddan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm on board with these changes! @Sean-Der sending over write-up now :)

@Sean-Der
Copy link
Member

Sean-Der commented Jun 1, 2024

Hi @tonisole

Sorry I didn't make more movement on this! I would love to see this get into pion/dtls

What do you think of making it non-PSK specific? I think this would be useful for certificates also?

@tonisole
Copy link
Author

tonisole commented Jun 4, 2024

Hi @Sean-Der,

Thank you for your feedback and interest in getting this feature into pion/dtls.

I’ve thought about your suggestion and have implemented a more general solution that is easier to implement and provides several benefits:

General Solution: The callback mechanism I’ve implemented is not specific to PSK. It can be used for any type of connection, including certificate-based ones. This makes it versatile and applicable to various scenarios.

Improved Performance: By placing the callback mechanism before the authorization validation, we can speed up the discard process. This means that connection attempts can be rejected early, reducing the load on the server and improving overall performance.

Backwards Compatibility: The best part of this solution is that it can be backported to the current version of DTLS without breaking changes. This ensures that existing implementations remain stable and functional while gaining the benefits of the new feature.

If you agree, I will cancel this Pull Request in favor of the newer #640 , more general, and easier-to-implement solution.

@tonisole tonisole closed this Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hasheddan hasheddan hasheddan left review comments

@at-wat at-wat at-wat left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tonisole @Sean-Der @at-wat @hasheddan